Prevent __has_builtin compiler differences from causing divergence
#7836
Conversation
michaelfig
force-pushed
the
mfig-i-can-haz-builtin
branch
from
06c6951
to
d22c9f8
Compare
There was a problem hiding this comment.
Sounds good, I see how it's a more reliable fix than the "assert __has_builtin() is available" that we added to xsnap-worker.c (a compiler which has __has_builtin() but lacks __builtin_add_overflow() would diverge under the xsnap-worker.c hack, but would fail to link with this PR, and a linker failure is the better outcome). And it's more reliable than a batch of assert __has_builtin(__builtin_add_overflow) checks at the start of xsnap-worker.c, because XS might start using new builtins in the future.
I think __has_builtin() is only used for potentially-builtin functions whose behavior and arguments are well-defined by the C specification. So if our override "return true" is at all different from the native version, the only difference will cause a linker error, not e.g. make XS call the function differently. It would not be safe to override a different sort of introspection function that had something to say about the argument signature.
|
|
||
| const hexHash = hash.digest('hex'); | ||
| t.log(hexHash); | ||
| t.snapshot(hexHash, description); |
There was a problem hiding this comment.
Let's add a note here to remind future test-runners of what we're asserting.
This test snapshot was created from a version of xsnap that was manually observed to avoid the spill (because it has the __builtin and can safely tell when it remains within the safe range of integers). We cannot test for the spill/not-spill directly without either parsing the xsnap heap snapshot, or writing C test code that performs a multiplication and then inspects the .kind field of the returned value. But we know that any change to the spill/non-spill behavior will change the heap snapshot hash.
The snapshot will change (and the test will fail) if either:
- XS is changed in a way that changes the spill behavior (e.g. maybe they give up on that optimization for some reason)
- XS is changed in any other way that changes the heap snapshot
It probably can't change in response to the compiler having/not-having the builtins, because the -D change means the not-having-builtins cases will simply fail to link, well before we get to this test case. And it won't change in response to the compiler having/not-having __has_builtin(), because the -D means we completely ignore the normal __has_builtin().
So as automated tests go, it's a bit too sensitive to things that 1: will happen, eventually, and 2: aren't what we're trying to assert. That's fine, especially since checking the real thing is too hard, but we should add a note, so when the test fails and the developer is considering ava's --update-snapshots, they can tell why this test was using a snapshot in the first place.
michaelfig
force-pushed
the
mfig-i-can-haz-builtin
branch
from
d22c9f8
to
a2b48e1
Compare
Prevent `__has_builtin` compiler differences from causing divergence
Prevent `__has_builtin` compiler differences from causing divergence
Prevent `__has_builtin` compiler differences from causing divergence
Prevent `__has_builtin` compiler differences from causing divergence
refs: #7829
refs: #7834
Description
As per #7829 (comment), I conducted an audit of
packages/xsnapused by the Agoric SDK, and found that of the 6 uses of__has_builtin, all of them were inpackages/xsnap/moddable, used to test for the presence of__builtin_XXX_overflow.Rather than feature-detection using
__has_builtinto choose between different implementations ofxsnapfunctionality, this PR redefines xsnap's__has_builtin(x)to always return nonzero, so that xsnap does not compile successfully, but with behaviour that has proven to diverge between different compilers.This change means the xsnap build will assume that all builtins are present, and thus it will fail-safe (with a linker error) if the specified builtin symbol is not defined by the compiler rather than fail-unsafe by using a different, potentially incompatible code path.
Security Considerations
n/a
Scaling Considerations
Slightly improves performance under GCC 9 (and some versions of clang), which omits
__has_builtinbut does implement__builtin_XXX_overflow.Documentation Considerations
May be able to document looser compiler chain/OS version requirements, if desirable.
Testing Considerations
Introduces a unit test to verify known problematic code does not cause the current platform's xsnap snapshots to diverge from expected hashes (generated on Ubuntu 22.04, amd64).
To run this test on your current platform:
(cd packages/xsnap && yarn test test/test-xsnap.js -m 'produce golden snapshot hashes')Before this
__has_builtinchange, the test passed on ubuntu-2022.4 (GCC 11, amd64 and arm64) and Darwin (clang 14, arm64) but failed the multiplication test on ubuntu-2020.4 (GCC 9, amd64 and arm64). After the__has_builtinchange, the test passed on all the platforms listed above.